Improve main #6053
Improve main #6053
Conversation
|
@matthias-ronge please rebase against current master, since there are code conflicts and this pull request is the next merge candidate. |
matthias-ronge
force-pushed
the
improve-main
branch
from
5df3a6d
to
e1156c6
Compare
There was a problem hiding this comment.
Just some small remarks. @matthias-ronge I know you just moved functionality to the new KitodoProduction.java file and didn't author it, but I think this is a good occasion to check the code for potential improvements, if they are low cost.
| startActiveMQ(); | ||
| } | ||
|
|
||
| private static final Optional<Manifest> retrieveManifestFileAsStream(ServletContext context) { |
There was a problem hiding this comment.
| private static final Optional<Manifest> retrieveManifestFileAsStream(ServletContext context) { | |
| private static Optional<Manifest> retrieveManifestFileAsStream(ServletContext context) { |
My IDE reports: "Such code might indicate an error or an incorrect assumption about the effect of the final keyword. Static methods are not subject to runtime polymorphism, so the only purpose of the final keyword used with static methods is to ensure the method will not be hidden in a subclass."
There was a problem hiding this comment.
I once did read that one should make all methods used in the constructor final, because if they are overloaded in a subclass, it could have confusing effects. I always do that; if the methods are under the constructor, you can see nicely in the “outline” what belongs to the constructors and where the actual class begins. For me, that is an improvement in the clarity of the code. In this case, it is particularly exciting because the “constructor” is not a constructor in the Java sense, but a method that is called by the Servlet Container to construct the class.
There was a problem hiding this comment.
I am not convinced having a structure outline in your IDE that you find more intuitive is a good enough reason to overrule this formal recommendation, but I can live with it if you really think it helps you.
| * | ||
| * @return the servlet context | ||
| */ | ||
| public ServletContext getServletContext() { |
There was a problem hiding this comment.
Do we need this method? My IDE shows it is unused, and the two interfaces that this method implements do not seem to require the method either.
There was a problem hiding this comment.
It's just a getter. You might want to get the servlet context to get the application's working directory, for example, or the server version. It's here for convenience. At the moment, it's not used yet.
There was a problem hiding this comment.
Please remove the method if it's unused and not required at the moment. Doesn't matter if it's just a getter. It should be added if required in the future.
| * | ||
| * @return the manifest | ||
| */ | ||
| public Optional<Manifest> getManifest() { |
There was a problem hiding this comment.
It is my idea that the main class of the application also provides essential characteristics of the application via getters. The manifest is such.
There was a problem hiding this comment.
Please remove unused getters if they are not technically required, see above.
matthias-ronge
force-pushed
the
improve-main
branch
from
0b7a6c4
to
709d406
Compare
matthias-ronge
force-pushed
the
improve-main
branch
from
709d406
to
7dc84a2
Compare
The main class was well hidden as
KitodoVersionListener. I renamed it toKitodoProductionand moved it. (Git doesn't get this on top of each other and show them as new, but it is from the previous classKitodoVersionListener. If you look at the individual commits, it becomes more understandable.) The instance exposes its essential properties, and the main class now handles essential functions of starting and stopping Active MQ and stopping the task manager, that was previously scattered throughout the application. The version is now no longer returned statically, but rather out of the main class.I find it tidier this way.